-
Notifications
You must be signed in to change notification settings - Fork 13.6k
Reland: [MLIR][Transforms] Fix Mem2Reg removal order to respect dominance #68877
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@llvm/pr-subscribers-mlir-core @llvm/pr-subscribers-mlir-openacc Author: Christian Ulmann (Dinistro) ChangesThis commit fixes a bug in the Mem2Reg operation erasure order. Replacing the use-def based topological order with a dominance-based weak order ensures that no operation is removed before all its uses have been replaced. The order relation uses the topological order of blocks and block internal ordering to determine a deterministic operation order. Additionally, the reliance on the Example:
When promoting the slot backing %ptr, it can happen that the LOAD0 was cleaned before LOAD1. This results in all uses of LOAD0 being replaced by its reaching definition, before LOAD1's result is replaced by LOAD0's result. The subsequent erasure of LOAD0 can thus not succeed, as it has remaining usages. Full diff: https://github.com/llvm/llvm-project/pull/68877.diff 11 Files Affected:
|
This is the third attempt of fixing this problem, previous commits did not have a correct strict weak ordering comparison function. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…ance This commit fixes a bug in the Mem2Reg operation erasure order. Replacing the use-def based topological order with a dominance-based weak order ensures that no operation is removed before all its uses have been replaced. The order relation uses the topological order of blocks and block internal ordering to determine a deterministic operation order. Additionally, the reliance on the `DenseMap` key order was eliminated by switching to a `MapVector`, that gives a deterministic iteration order. Example: ``` %ptr = alloca ... ... %val0 = %load %ptr ... // LOAD0 store %val0 %ptr ... %val1 = load %ptr ... // LOAD1 ```` When promoting the slot backing %ptr, it can happen that the LOAD0 was cleaned before LOAD1. This results in all uses of LOAD0 being replaced by its reaching definition, before LOAD1's result is replaced by LOAD0's result. The subsequent erasure of LOAD0 can thus not succeed, as it has remaining usages.
0b33cf6
to
726f54c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This commit fixes a bug in the Mem2Reg operation erasure order. Replacing the use-def based topological order with a dominance-based weak order ensures that no operation is removed before all its uses have been replaced. The order relation uses the topological order of blocks and block internal ordering to determine a deterministic operation order.
Additionally, the reliance on the
DenseMap
key order was eliminated by switching to aMapVector
, that gives a deterministic iteration order.Example:
When promoting the slot backing %ptr, it can happen that the LOAD0 was cleaned before LOAD1. This results in all uses of LOAD0 being replaced by its reaching definition, before LOAD1's result is replaced by LOAD0's result. The subsequent erasure of LOAD0 can thus not succeed, as it has remaining usages.